Fix cppcheck warnings, typos, style issues#1227
Fix cppcheck warnings, typos, style issues#1227iche033 merged 2 commits intogazebosim:gz-rendering10from
Conversation
There was a problem hiding this comment.
Follow-up questions:
A:
Fix for next warnings or can break ABI or need some clarification. I'd like to discuss it separately and PR(s) can be done in main for example:
-
Member variable 'MapFunctionEnum::variants' is not assigned a value in 'MapFunctionEnum::operator='.
Not clear, is it intentional or mistake. If it's intentional, maybe better left comment. Or maybe make it not copy assignable(just move). Or just use=defaultif it's mistake -
Class 'SceneExt' which has virtual members does not have a virtual destructor.
gz-rendering/include/gz/rendering/base/SceneExt.hh:45
(change ABI, but seems should be changed) -
The class 'BaseParticleEmitter < Ogre2Visual >' defines member variable with name 'material' also defined in its parent class 'BaseVisual < Ogre2Node >'.
(change ABI, but seems should be removed one, or maybe it should be 2 different and need another name in this case) -
Hiding warning in cppcheck, maybe
virtualis missing?
The class 'OgreCOMVisual' defines member function with name 'OgreObject' also defined in its parent class 'OgreObject'.
The class 'OgreInertiaVisual' defines member function with name 'OgreObject' also defined in its parent class 'OgreObject'.
The class 'OgreLightVisual' defines member function with name 'OgreObject' also defined in its parent class 'OgreObject'.
B:
What do you think about adding GZ_PROFILER for renders? It can have more realistic picture of performance problem with rendering sensors or GUI part? I mean, I can do it, just want to check that this change can be accepted to the codebase. And which branch should be targeted. We are using stable release to be honest.
| ///////////////////////////////////////////////// | ||
| gz::math::AxisAlignedBox transformAxisAlignedBox( | ||
| const gz::math::AxisAlignedBox &_bbox, | ||
| const gz::math::AxisAlignedBox &_box, |
There was a problem hiding this comment.
seems _box is more common in this repo
| for (unsigned int i = 0; i < 6; ++i) | ||
| { | ||
| this->ogreCompositorWorkspace[i] = nullptr; | ||
| } |
There was a problem hiding this comment.
CppCheck warning, technically it was initialized in other place, just moved it from parent's constructor. Technically same result can get in a shorter way, not only explicit code. But left it as is for simplicity.
| size_t idx = meshName.find("::"); | ||
| if (idx != std::string::npos) | ||
| meshName = meshName.substr(0, idx); | ||
| meshName.resize(idx); |
There was a problem hiding this comment.
also cppcheck warning;
live example: https://godbolt.org/z/9vfKnEqav
| "The number of supported emitters does not match the number of " | ||
| "Ogre emitter types."); | ||
|
|
||
| GZ_ASSERT(this->type < kOgreEmitterTypes.size(), "Unknown emitter type"); |
There was a problem hiding this comment.
in other method similar check exist. So it's nice to have it here too. Not very accessible outside, so it should be safe to have assert and not runtime return.
| math::Vector3d ray(0, 0, 1); | ||
| ray.Normalize(); |
There was a problem hiding this comment.
Seems it should be the same unit vector after Normalize. Seems we can get rid of this call
| int index = 0; | ||
| for (unsigned int i = 0; i < this->dataPtr->h2nd; ++i) | ||
| { | ||
| const math::Quaterniond pitch(math::Vector3d(1, 0, 0), -v); |
There was a problem hiding this comment.
Invariant moved out of the nested loop
| #ifndef GZ_RENDERING_OGRE2_OGRE2GPURAYS_HH_ | ||
| #define GZ_RENDERING_OGRE2_OGRE2GPURAYS_HH_ | ||
|
|
||
| #include <functional> |
There was a problem hiding this comment.
for std::function, guess should be fixed in multiple places, better with include-what-you-use but not in this PR
ntfshard
force-pushed
the
variousfixes
branch
2 times, most recently
from
3c5b6c0 to
65506ed
Compare
Signed-off-by: Maksim Derbasov <ntfs.hard@gmail.com>
ntfshard
force-pushed
the
variousfixes
branch
from
65506ed to
88e9368
Compare
|
Thanks for the fixes! Changes look good to me.
Looking at the code, I think
yes, we can add virtual destructor but in
For this particular class, we could remove the
yes looks like they should be overriding the functions from base class
I think it's a good idea. Good to provide more finer profiling points with |
|
@ntfshard Thanks for the PR! Since we use the PR title as the changelog entry, could you please make it more descriptive? |
But it will be copied in case of copy constructor: https://godbolt.org/z/KMeYonPKh
Render is still quite isolated part but if someone using it through parent type, it should looks like not working. I guess we can clarify details in a follow-up PR
I'll change it. I'm not native English speaker, feel free to correct it for better naming Just noticed, warning with And about I'll put small PR for virtual dtor to |
To make behaviour more clear for further development, left comment and disabled copy constructor |
Signed-off-by: Maksim Derbasov <ntfs.hard@gmail.com>
yes that inheritance architecture is something I've considered refactoring / removing for a while (related to #22). It's a big effort which unfortunately we didn't have time to do.
looks good to me |
|
I'll merge this first as it's already a big PR. Other changes can come in follow-up PRs. |
|
@Mergifyio backport main gz-rendering9 gz-rendering8 |
✅ Backports have been createdDetails
Cherry-pick of a395a95 has failed: To fix up this pull request, you can check it out locally. See documentation: https://docs.github.com/en/pull-requests/collaborating-with-pull-requests/reviewing-changes-in-pull-requests/checking-out-pull-requests-locally |
Reducing CppCheck report size, frequent warnings: * missing override (CI-clang seems also affected by this) * different argument name * variable scope can be reduced * Ogre or containers in standard library most of times use size_t, better to use * Fix typos * In general tried to follow the rule from c++ core guidelines https://isocpp.github.io/CppCoreGuidelines/CppCoreGuidelines#c128-virtual-functions-should-specify-exactly-one-of-virtual-override-or-final (but changing it in all code could make PR much bigger) Signed-off-by: Maksim Derbasov <ntfs.hard@gmail.com> (cherry picked from commit a395a95)
Reducing CppCheck report size, frequent warnings: * missing override (CI-clang seems also affected by this) * different argument name * variable scope can be reduced * Ogre or containers in standard library most of times use size_t, better to use * Fix typos * In general tried to follow the rule from c++ core guidelines https://isocpp.github.io/CppCoreGuidelines/CppCoreGuidelines#c128-virtual-functions-should-specify-exactly-one-of-virtual-override-or-final (but changing it in all code could make PR much bigger) Signed-off-by: Maksim Derbasov <ntfs.hard@gmail.com> (cherry picked from commit a395a95)
Reducing CppCheck report size, frequent warnings: * missing override (CI-clang seems also affected by this) * different argument name * variable scope can be reduced * Ogre or containers in standard library most of times use size_t, better to use * Fix typos * In general tried to follow the rule from c++ core guidelines https://isocpp.github.io/CppCoreGuidelines/CppCoreGuidelines#c128-virtual-functions-should-specify-exactly-one-of-virtual-override-or-final (but changing it in all code could make PR much bigger) Signed-off-by: Maksim Derbasov <ntfs.hard@gmail.com> (cherry picked from commit a395a95) # Conflicts: # ogre/src/OgreFrustumVisual.cc # ogre2/include/gz/rendering/ogre2/Ogre2Scene.hh
Reducing CppCheck report size, frequent warnings: * missing override (CI-clang seems also affected by this) * different argument name * variable scope can be reduced * Ogre or containers in standard library most of times use size_t, better to use * Fix typos * In general tried to follow the rule from c++ core guidelines https://isocpp.github.io/CppCoreGuidelines/CppCoreGuidelines#c128-virtual-functions-should-specify-exactly-one-of-virtual-override-or-final (but changing it in all code could make PR much bigger) Signed-off-by: Maksim Derbasov <ntfs.hard@gmail.com> (cherry picked from commit a395a95)
Reducing CppCheck report size, frequent warnings: * missing override (CI-clang seems also affected by this) * different argument name * variable scope can be reduced * Ogre or containers in standard library most of times use size_t, better to use * Fix typos * In general tried to follow the rule from c++ core guidelines https://isocpp.github.io/CppCoreGuidelines/CppCoreGuidelines#c128-virtual-functions-should-specify-exactly-one-of-virtual-override-or-final (but changing it in all code could make PR much bigger) Signed-off-by: Maksim Derbasov <ntfs.hard@gmail.com> (cherry picked from commit a395a95)
Signed-off-by: Ian Chen <ichen@openrobotics.org>
Signed-off-by: Ian Chen <ichen@openrobotics.org>
3 participants
🦟 Bug fix
Fixes #
Summary
size_t, better to useI'm running CppCheck with
compile_commands.jsonfile which generated bycmake -DCMAKE_EXPORT_COMPILE_COMMANDS=ON(and manual path tweak to run scan on host), notmake cppcheckChecklist
codecheckpassed (See contributing)Note to maintainers: Remember to use Squash-Merge and edit the commit message to match the pull request summary while retaining
Signed-off-byandGenerated-bymessages.